-
Notifications
You must be signed in to change notification settings - Fork 458
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
detect clang/gcc using --version #709
Conversation
@thomcc This patch is broken after rebase. Could you tell me if it is worth to fix it or not? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think this is fine. Sorry for the delay. Can you fix the failures?
@@ -1663,6 +1663,16 @@ impl Build { | |||
family.add_force_frame_pointer(cmd); | |||
} | |||
|
|||
if !cmd.is_like_msvc() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems unrelated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember the details anymore, but they are related to tests gnu_i686
and gnu_x86_64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I got the idea. This happened because gcc()
now target x86_64-apple-darwin
on macOS.
I can revert this part and add target to gnu_i686
and gnu_x86_64
. Is this better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By itself it's a formally wrong move. While the -m32/m64 are accepted by clang, the --target argument [which is always passed to clang by cc-rs] describes the intention sufficiently and more importantly more adequately. In other words the options in question should have remained in the ToolFamily::Gnu section. If they cause problems in the section in question, then it would have been more appropriate to solve it there.
On a tangential note. The powepc64 gcc defaults to -m64, so that it's actually more interesting to pass -m32 than -m64. But of course making it symmetric would be better, i.e. pass -m32 if the target is 32-bit ppc, and -m64 otherwise...
1c12087
to
f51788d
Compare
Co-authored-by: Thom Chiovoloni <[email protected]>
f51788d
to
cd3695a
Compare
@thomcc Tests are fixed. Could you review it again? |
Maybe replacement of #407 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me.
It looks like this broke CI, hmmm... |
This is useful on macos because
gcc
andcc
is actuallyclang
.gcc
andcc
were regarded asclang
even when it was gcc.m32
andm64
is not necessary for clang, but also harmless.Do you want to do it as now or want to revert it and change tests run only for gcc?